[devel] feat: implement rolling pilot log output#155
Conversation
|
@sfayer Forgot to tag you. |
|
I'm getting the full command buffer from the command execution at the moment, but it could be modified, I think. I can flush my buffer on demand as well, not after certain number of lines. |
|
I can, as anyway this is targeting |
|
I'll deal with it after my next meeting, but before the end of today. |
|
@marianne013: from testing, we think this patch is also needed: sys.stdout.write("\n")
sys.stdout.flush()
+ sys.stderr.write("\n")
+ sys.stderr.flush()Regards, |
|
I was kind of hoping it would let me click the "implement suggestion" button, but alas, no |
martynia
left a comment
martynia
left a comment
There was a problem hiding this comment.
The code works in"real life" in our test v7r3 installation, however I'm not sure if python 2 is not a limitation in this scenario.
| sys.stderr.write("\n") | ||
| sys.stderr.flush() | ||
| # Decode full buffer to ascii (to avoid codepoint splitting problems) | ||
| outData = outData.decode("ascii", "replace") |
There was a problem hiding this comment.
Since outData and all the chunks are strings, outData.decode will not work in Python 3. I added a little unit test in #158, which passes when run in Python2 only.
There was a problem hiding this comment.
Can you be slightly more precise as in which test is it that fails and what the error is ?
There was a problem hiding this comment.
You would need to check #158 out, and then:
python2 -m pytest tests/CI/Test_simplePilotLogger.py
this would pass. If you run the same test with Python 3 it will fail. This is because outData is a string and doesn't have a decode() method in Python 3. You cannot change everything to bytes to satisfy line #428, because writing to stdout/stderr requires strings (add appending chunks to outData requires strings), so outData must be a string.
You can simply run in python 2 and 3 and see what happens:
text="Hello World"
text.decode('acsii','replace')There was a problem hiding this comment.
I extended my test to run it for a bunch of strings with different lengths (not pushed yet). It seemed to run fine, but after I repeated the test for a few times (from a command line, as shown above), I got a failure. It was somewhat difficult to see where the strings were different (at 1025 length...). A closer look (text is a random ASCII letter sequence + \n ) \
- the original string starts with a newline character, the string processed by the program doesn't (it is now 1024 characters long)
So it is missing a leading newline character. Not the end of the world, we can live with it, I can exclude these cases from a test.
There was a problem hiding this comment.
Where are we with this? We're really keen to get this rolling output in as it's hampering our attempts to debug other problems as we have to wait for the JobAgent to finish to get its log output...
Regards,
Simon
There was a problem hiding this comment.
Can you live with the fact, that the code does not work with Python 3 ? See my comments above to Daniela's post.
There was a problem hiding this comment.
Can you live with the fact, that the code does not work with Python 3 ? See my comments above to Daniela's post.
no :)
E AttributeError: 'str' object has no attribute 'decode'
|
Nah, stream.read() is bytes on py3, str on py2 (which is how the old code works with both)... We're still running into a few minor unicode problems, but we think that should generally fix it. |
marianne013
force-pushed
the
devel
branch
2 times, most recently
from
4951efb to
5805e0f
Compare
|
We've re-tested this latest version with both py3 & py2 and it works for both.... Any other comments? Regards, |
martynia
left a comment
martynia
left a comment
There was a problem hiding this comment.
Tested with Python 2 and 3. OK.
|
Hi @fstagni, I think this PR is all ready to go now, do you need anything further from us to get it merged? Regards, |
|
I think we could go ahead. I’ll deal with this change in my remote logger, once it is in.
Cheers, JM
… On 21 Jun 2022, at 15:57, fstagni ***@***.***> wrote:
I can, as anyway this is targeting devel branch and so it will go through a hackathon. It's still a draft though.
—
Reply to this email directly, view it on GitHub <#155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2KE7K2WTPHQDSXL46EDKDVQHC3RANCNFSM5XD6R3QQ>.
You are receiving this because you were mentioned.
|
4 participants
@fstagni @martynia
Simon had a go at trying to make the pilot write its log file on a rolling basis. I tested it and as far as I can tell it doesn't break anything.
This might be a good stepping stone towards handling the output in the pilot logger.